Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Flatten the 'db' commands, e.g. 'db prefix' to 'db/prefix' #69

Merged
merged 1 commit into from
Nov 14, 2024
Merged

Flatten the 'db' commands, e.g. 'db prefix' to 'db/prefix' #69

merged 1 commit into from
Nov 14, 2024

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Nov 11, 2024

The user experience with commands that have sub-commands is not great and it makes it harder to implement completion in 'cilium-dbg shell' if we so choose. There isn't really any good rational for nesting the commands like this, so let's go with Plan 9 style command names (https://9p.io/magic/man2html/8/auth), e.g. "db prefix" becomes "db/prefix".

@joamaki joamaki requested a review from a team as a code owner November 11, 2024 15:39
@joamaki joamaki requested review from pippolo84 and removed request for a team November 11, 2024 15:39
@joamaki
Copy link
Contributor Author

joamaki commented Nov 11, 2024

I went to fair bit of effort in #65 to document the "sub-commands" and I just realized today that it's kind of stupid to nest the commands like this. It makes them hard to find (help wont list them) and it makes it hard/impossible to implement command-line completion in cilium-dbg shell if we want to add one later. The script/shell thing should be as simple and dumb as possible, so let's simplify this.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 11, 2024 •
edited
Loading 

$ make test
go: downloading golang.org/x/sys v0.17.0
go: downloading golang.org/x/tools v0.17.0
go: downloading golang.org/x/exp v0.0.0-20240119083558-1b970713d09a
go: downloading github.com/fsnotify/fsnotify v1.7.0
go: downloading github.com/sagikazarmark/slog-shim v0.1.0
go: downloading github.com/spf13/afero v1.11.0
go: downloading github.com/spf13/cast v1.6.0
go: downloading github.com/subosito/gotenv v1.6.0
go: downloading github.com/hashicorp/hcl v1.0.0
go: downloading gopkg.in/ini.v1 v1.67.0
go: downloading github.com/magiconair/properties v1.8.7
go: downloading github.com/pelletier/go-toml/v2 v2.1.0
go: downloading golang.org/x/text v0.14.0
	github.com/cilium/statedb/reconciler/benchmark		coverage: 0.0% of statements
	github.com/cilium/statedb/reconciler/example		coverage: 0.0% of statements
ok  	github.com/cilium/statedb	29.988s	coverage: 82.7% of statements
ok  	github.com/cilium/statedb/index	0.006s	coverage: 28.7% of statements
ok  	github.com/cilium/statedb/internal	0.010s	coverage: 46.7% of statements
ok  	github.com/cilium/statedb/part	2.744s	coverage: 83.2% of statements
ok  	github.com/cilium/statedb/reconciler	0.279s	coverage: 89.7% of statements
-----
$ make bench
go test ./... -bench . -benchmem -test.run xxx
goos: linux
goarch: amd64
pkg: github.com/cilium/statedb
cpu: AMD EPYC 7763 64-Core Processor                
BenchmarkDB_WriteTxn_1-4                    	  444084	      2699 ns/op	    370544 objects/sec	    2816 B/op	      32 allocs/op
BenchmarkDB_WriteTxn_10-4                   	 1214216	       984.2 ns/op	   1016021 objects/sec	     743 B/op	      10 allocs/op
BenchmarkDB_WriteTxn_100-4                  	 1555790	       766.6 ns/op	   1304406 objects/sec	     597 B/op	       7 allocs/op
BenchmarkDB_WriteTxn_1000-4                 	 1460091	       830.2 ns/op	   1204556 objects/sec	     549 B/op	       7 allocs/op
BenchmarkDB_WriteTxn_100_SecondaryIndex-4   	  503707	      2323 ns/op	    430449 objects/sec	    1505 B/op	      37 allocs/op
BenchmarkDB_Modify-4                        	    1267	    937209 ns/op	   1066999 objects/sec	  774498 B/op	    8463 allocs/op
BenchmarkDB_GetInsert-4                     	    1194	   1010853 ns/op	    989265 objects/sec	  755673 B/op	    8461 allocs/op
BenchmarkDB_RandomInsert-4                  	    2355	    501711 ns/op	   1993180 objects/sec	  402233 B/op	    7097 allocs/op
BenchmarkDB_RandomReplace-4                 	     310	   3836403 ns/op	    260661 objects/sec	 2357567 B/op	   48575 allocs/op
BenchmarkDB_SequentialInsert-4              	    1494	    804130 ns/op	   1243580 objects/sec	  551363 B/op	    7285 allocs/op
BenchmarkDB_Changes_Baseline-4              	    1244	    957850 ns/op	   1044006 objects/sec	  553134 B/op	   10252 allocs/op
BenchmarkDB_Changes-4                       	     679	   1755950 ns/op	    569492 objects/sec	  993403 B/op	   14493 allocs/op
BenchmarkDB_RandomLookup-4                  	   22114	     53786 ns/op	  18592125 objects/sec	     160 B/op	       1 allocs/op
BenchmarkDB_SequentialLookup-4              	   27476	     44156 ns/op	  22647015 objects/sec	       0 B/op	       0 allocs/op
BenchmarkDB_Prefix_SecondaryIndex-4         	    7539	    149192 ns/op	   6702759 objects/sec	   93911 B/op	    1044 allocs/op
BenchmarkDB_FullIteration_All-4             	     672	   1727447 ns/op	  57889034 objects/sec	     480 B/op	      12 allocs/op
BenchmarkDB_FullIteration_Get-4             	     204	   5926269 ns/op	  16874053 objects/sec	       0 B/op	       0 allocs/op
BenchmarkDB_PropagationDelay-4              	  511288	      2395 ns/op	        20.00 50th_µs	        25.00 90th_µs	        90.00 99th_µs	    1590 B/op	      24 allocs/op
PASS
ok  	github.com/cilium/statedb	27.948s
PASS
ok  	github.com/cilium/statedb/index	0.004s
PASS
ok  	github.com/cilium/statedb/internal	0.003s
goos: linux
goarch: amd64
pkg: github.com/cilium/statedb/part
cpu: AMD EPYC 7763 64-Core Processor                
Benchmark_Insert_RootOnlyWatch-4    	    8776	    136047 ns/op	   7350388 objects/sec	  104164 B/op	    2041 allocs/op
Benchmark_Insert-4                  	    6258	    186430 ns/op	   5363932 objects/sec	  219055 B/op	    3064 allocs/op
Benchmark_Modify-4                  	    7784	    152581 ns/op	   6553923 objects/sec	  212697 B/op	    1205 allocs/op
Benchmark_GetInsert-4               	    6507	    187044 ns/op	   5346342 objects/sec	  212401 B/op	    1203 allocs/op
Benchmark_Replace-4                 	26830669	        44.17 ns/op	  22641985 objects/sec	       0 B/op	       0 allocs/op
Benchmark_Replace_RootOnlyWatch-4   	26879574	        44.71 ns/op	  22366624 objects/sec	       0 B/op	       0 allocs/op
Benchmark_txn_1-4                   	 2935118	       426.6 ns/op	   2344373 objects/sec	     448 B/op	       7 allocs/op
Benchmark_txn_10-4                  	 7056416	       168.2 ns/op	   5945356 objects/sec	     154 B/op	       2 allocs/op
Benchmark_txn_100-4                 	 7776567	       155.4 ns/op	   6434067 objects/sec	     224 B/op	       2 allocs/op
Benchmark_txn_1000-4                	 6955147	       168.6 ns/op	   5931286 objects/sec	     216 B/op	       2 allocs/op
Benchmark_txn_delete_1-4            	 3241438	       371.1 ns/op	   2694992 objects/sec	     856 B/op	       6 allocs/op
Benchmark_txn_delete_10-4           	 8821153	       138.9 ns/op	   7199447 objects/sec	     132 B/op	       1 allocs/op
Benchmark_txn_delete_100-4          	10738854	       108.8 ns/op	   9191937 objects/sec	      60 B/op	       1 allocs/op
Benchmark_txn_delete_1000-4         	11706182	       103.3 ns/op	   9684056 objects/sec	      26 B/op	       1 allocs/op
Benchmark_Get-4                     	   36836	     31712 ns/op	  31533869 objects/sec	       0 B/op	       0 allocs/op
Benchmark_Iterate-4                 	  167732	      7399 ns/op	 135144889 objects/sec	      80 B/op	       3 allocs/op
Benchmark_Hashmap_Insert-4          	   15656	     75771 ns/op	  13197731 objects/sec	   86549 B/op	      64 allocs/op
Benchmark_Hashmap_Get_Uint64-4      	  143612	      8419 ns/op	 118778410 objects/sec	       0 B/op	       0 allocs/op
Benchmark_Hashmap_Get_Bytes-4       	  152944	      7862 ns/op	 127190274 objects/sec	       0 B/op	       0 allocs/op
Benchmark_Uint64Map_Random-4        	    1327	    886615 ns/op	   1127886 items/sec	 2700474 B/op	    9024 allocs/op
Benchmark_Uint64Map_Sequential-4    	    1466	    830707 ns/op	   1203795 items/sec	 2492408 B/op	    9749 allocs/op
PASS
ok  	github.com/cilium/statedb/part	28.558s
PASS
ok  	github.com/cilium/statedb/reconciler	0.005s
?   	github.com/cilium/statedb/reconciler/benchmark	[no test files]
?   	github.com/cilium/statedb/reconciler/example	[no test files]
go run ./reconciler/benchmark -quiet
1000000 objects reconciled in 3.00 seconds (batch size 1000)
Throughput 332939.68 objects per second
Allocated 6011330 objects, 424778kB bytes, 525464kB bytes still in use

ovidiutirla
ovidiutirla approved these changes Nov 12, 2024
View reviewed changes
Copy link

@ovidiutirla ovidiutirla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

pippolo84
pippolo84 approved these changes Nov 12, 2024
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see that the (pretty complicated) logic to generate the usage details has gone away. I'm (mildly) concerned that the Plan 9 style might be kind of unexpected when using statedb in cilium where we rely on subcommands, but I'd say that it's worth to give this a try. 👍

@joamaki joamaki force-pushed the pr/joamaki/flatten-db-commands branch from b627011 to b94aa80 Compare November 14, 2024 14:54
@joamaki
Flatten the 'db' commands, e.g. 'db prefix' to 'db/prefix'
be4b5c2 
The user experience with commands that have sub-commands is not great
and it makes it harder to implement completion in 'cilium-dbg shell'
if we so choose. There isn't really any good rational for nesting
the commands like this, so let's go with Plan 9 style command names
(https://9p.io/magic/man2html/8/auth), e.g. "db prefix" becomes
"db/prefix".

Signed-off-by: Jussi Maki <[email protected]>
@joamaki joamaki force-pushed the pr/joamaki/flatten-db-commands branch from b94aa80 to be4b5c2 Compare November 14, 2024 15:17
@joamaki joamaki merged commit b234be5 into main Nov 14, 2024
1 check passed
@joamaki joamaki deleted the pr/joamaki/flatten-db-commands branch November 14, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ovidiutirla ovidiutirla ovidiutirla approved these changes

@pippolo84 pippolo84 pippolo84 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joamaki @pippolo84 @ovidiutirla